feat(ingestion): Adding ability to ignore users from top users calculation #3735

treff7es · 2021-12-14T08:58:49Z

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable)

treff7es · 2021-12-14T09:00:59Z

Here is an example how it looks like if you add the following to the usage config and only that user queried the table:

  ignore_user_emails_from_usage:
    - [email protected]

Every other stat contains those queries except the top users.

github-actions · 2021-12-14T09:26:22Z

Unit Test Results

    45 files +  1     45 suites +1 45m 1s ⏱️ + 2m 45s
  650 tests +56   598 ✔️ +56 52 💤 ±0 0 ❌ ±0
1 472 runs +88 1 398 ✔️ +88 74 💤 ±0 0 ❌ ±0

Results for commit 7045348. ± Comparison against base commit 22cef5f.

♻️ This comment has been updated with latest results.

rslanka · 2022-01-27T19:56:35Z

metadata-ingestion/src/datahub/ingestion/source/usage/usage_common.py

@@ -112,6 +116,7 @@ def make_usage_workunit(

 class BaseUsageConfig(BaseTimeWindowConfig):
    top_n_queries: pydantic.PositiveInt = 10
+    ignore_user_emails_from_usage: Optional[List[str]]


Can we make this an AllowDenyPattern type to be consistent with rest of the pattern based filters we have?

You are right, I changed it.

rslanka · 2022-01-27T19:58:35Z

metadata-ingestion/src/datahub/ingestion/source/usage/usage_common.py

@@ -27,6 +27,7 @@
 class GenericAggregatedDataset(Generic[ResourceType]):
    bucket_start_time: datetime
    resource: ResourceType
+    ignore_user_emails: Optional[List[str]] = None


change it to an AllowDenyPattern type.

rslanka · 2022-01-27T20:00:35Z

metadata-ingestion/tests/unit/test_usage_common_tmp.py

@@ -0,0 +1,174 @@
+from datetime import datetime


Is this added by mistake? Can we merge this with test_usage_common.py?

Yeah, it was a mistake. I removed it.

rslanka · 2022-01-27T20:12:32Z

metadata-ingestion/src/datahub/ingestion/source/usage/usage_common.py

-        self.userFreq[user_email] += 1
+        if not self.ignore_user_emails or user_email not in self.ignore_user_emails:
+            self.userFreq[user_email] += 1
+
        if query:


Why are we not dropping the query made by an ignored user? Should we not just drop the entry altogether, if we don't want the data from the user pollute the usage stats?

To add in here, when we first discussed this feature we agreed to not drop them from totals as it was a heavier touch operation. I think we have enough signal to know that it would make sense to drop these users from sum total as well.

I updated the logic to skip these users from every count.

github-actions · 2022-01-28T09:00:24Z

Unit Test Results (build & test)

  66 files ±0   66 suites ±0 10m 52s ⏱️ - 2m 1s
581 tests ±0 522 ✔️ ±0 59 💤 ±0 0 ❌ ±0

Results for commit 5748cb9. ± Comparison against base commit ff367c0.

♻️ This comment has been updated with latest results.

github-actions · 2022-01-28T09:06:59Z

Unit Test Results (metadata ingestion)

      5 files ±  0       5 suites ±0 39m 10s ⏱️ -51s
  279 tests +  2   279 ✔️ +  2   0 💤 ±0 0 ❌ ±0
1 284 runs +10 1 255 ✔️ +10 29 💤 ±0 0 ❌ ±0

Results for commit 5748cb9. ± Comparison against base commit ff367c0.

♻️ This comment has been updated with latest results.

rslanka · 2022-01-28T17:13:29Z

metadata-ingestion/src/datahub/ingestion/source/usage/usage_common.py

@@ -27,6 +28,7 @@
 class GenericAggregatedDataset(Generic[ResourceType]):
    bucket_start_time: datetime
    resource: ResourceType
+    ignore_user_emails: AllowDenyPattern = AllowDenyPattern.allow_all()


s/ignore_user_emails/user_email_pattern/. The deny list will imply ignore.

Corresponding documentation updates.

rslanka · 2022-01-28T17:15:11Z

metadata-ingestion/tests/unit/test_usage_common.py

+    test_email2 = "[email protected]"
+    test_query = "select * from test"
+    test_query2 = "select * from test2"
+


nit: remove unnecessary new lines here.

rslanka · 2022-01-28T17:15:33Z

metadata-ingestion/tests/unit/test_usage_common.py

+def test_add_one_query_with_ignored_user():
+    test_email = "[email protected]"
+    test_query = "select * from test"
+


nit: remove unnecessary new lines here.

rslanka · 2022-01-28T17:16:27Z

metadata-ingestion/source_docs/trino.md

+| `start_time`                               |          | Last full day in UTC (or hour, depending on `bucket_duration`) | Earliest date of usage logs to consider.                        |
+| `end_time`                                 |          | Last full day in UTC (or hour, depending on `bucket_duration`) | Latest date of usage logs to consider.                          |
+| `top_n_queries`                            |          | `10`                                                           | Number of top queries to save to each table.                    |
+| `ignore_user_emails_from_usage.allow`      |          |                                                                | List of regex patterns for user emails to include in usage.     |


s/ignore_user_emails_from_usage/user_email_pattern.

rslanka · 2022-01-28T20:03:43Z

metadata-ingestion/src/datahub/ingestion/source/usage/bigquery_usage.py

+                AggregatedDataset(
+                    bucket_start_time=floored_ts,
+                    resource=resource,
+                    ignore_user_emails=self.config.ignore_user_emails_from_usage,


Haven't these gotten renamed?

rslanka · 2022-01-28T20:03:59Z

metadata-ingestion/src/datahub/ingestion/source/usage/redshift_usage.py

+                AggregatedDataset(
+                    bucket_start_time=floored_ts,
+                    resource=resource,
+                    ignore_user_emails=self.config.ignore_user_emails_from_usage,


Haven't these gotten renamed?

rslanka · 2022-01-28T20:04:05Z

metadata-ingestion/src/datahub/ingestion/source/usage/snowflake_usage.py

+                    AggregatedDataset(
+                        bucket_start_time=floored_ts,
+                        resource=resource,
+                        ignore_user_emails=self.config.ignore_user_emails_from_usage,


Haven't these gotten renamed?

rslanka · 2022-01-28T20:04:13Z

metadata-ingestion/src/datahub/ingestion/source/usage/starburst_trino_usage.py

-                            bucket_start_time=floored_ts, resource=resource
+                            bucket_start_time=floored_ts,
+                            resource=resource,
+                            ignore_user_emails=self.config.ignore_user_emails_from_usage,


Haven't these gotten renamed?

rslanka

LGTM!

shirshanka

LGTM

treff7es force-pushed the ignore_emails_from_top_users branch 2 times, most recently from 580b599 to 7045348 Compare December 17, 2021 07:32

rslanka reviewed Jan 27, 2022

View reviewed changes

treff7es closed this Jan 28, 2022

treff7es reopened this Jan 28, 2022

rslanka reviewed Jan 28, 2022

View reviewed changes

rslanka approved these changes Jan 31, 2022

View reviewed changes

treff7es added 2 commits January 31, 2022 23:25

Addressing pr review comments

3068532

Fixing rebase changes

5748cb9

treff7es force-pushed the ignore_emails_from_top_users branch from 9e4b166 to 5748cb9 Compare January 31, 2022 22:31

shirshanka approved these changes Feb 1, 2022

View reviewed changes

shirshanka merged commit 6871122 into datahub-project:master Feb 1, 2022

treff7es deleted the ignore_emails_from_top_users branch February 8, 2023 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ingestion): Adding ability to ignore users from top users calculation #3735

feat(ingestion): Adding ability to ignore users from top users calculation #3735

treff7es commented Dec 14, 2021

treff7es commented Dec 14, 2021 •

edited

Loading

github-actions bot commented Dec 14, 2021 •

edited

Loading

rslanka Jan 27, 2022

treff7es Jan 28, 2022

rslanka Jan 27, 2022

rslanka Jan 27, 2022

treff7es Jan 28, 2022

rslanka Jan 27, 2022

gabe-lyons Jan 27, 2022

treff7es Jan 28, 2022

github-actions bot commented Jan 28, 2022 •

edited

Loading

github-actions bot commented Jan 28, 2022 •

edited

Loading

rslanka Jan 28, 2022 •

edited

Loading

rslanka Jan 28, 2022

rslanka Jan 28, 2022

rslanka Jan 28, 2022

rslanka Jan 28, 2022

rslanka Jan 28, 2022

rslanka Jan 28, 2022

rslanka Jan 28, 2022

rslanka left a comment

shirshanka left a comment

feat(ingestion): Adding ability to ignore users from top users calculation #3735

feat(ingestion): Adding ability to ignore users from top users calculation #3735

Conversation

treff7es commented Dec 14, 2021

Checklist

treff7es commented Dec 14, 2021 • edited Loading

github-actions bot commented Dec 14, 2021 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 28, 2022 • edited Loading

Unit Test Results (build & test)

github-actions bot commented Jan 28, 2022 • edited Loading

Unit Test Results (metadata ingestion)

rslanka Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rslanka left a comment

Choose a reason for hiding this comment

shirshanka left a comment

Choose a reason for hiding this comment

treff7es commented Dec 14, 2021 •

edited

Loading

github-actions bot commented Dec 14, 2021 •

edited

Loading

github-actions bot commented Jan 28, 2022 •

edited

Loading

github-actions bot commented Jan 28, 2022 •

edited

Loading

rslanka Jan 28, 2022 •

edited

Loading